Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Fly.io provider #1

Merged
merged 1 commit into from
Jun 5, 2024
Merged

Add Fly.io provider #1

merged 1 commit into from
Jun 5, 2024

Conversation

bcmmbaga
Copy link
Collaborator

Add Fly.io Provider

Description

This pull request adds the implementation of Fly.io as a Daytona container provider.

  • This change requires a documentation update
  • I have made corresponding changes to the documentation

pkg/types/targets.go Show resolved Hide resolved
pkg/types/metadata.go Show resolved Hide resolved
pkg/provider/provider_test.go Show resolved Hide resolved
pkg/provider/provider.go Outdated Show resolved Hide resolved
@bcmmbaga bcmmbaga force-pushed the add-fly-provider branch 2 times, most recently from aaaa544 to aa461f5 Compare May 28, 2024 18:23
@bcmmbaga bcmmbaga requested a review from Tpuljak May 28, 2024 18:24
Copy link
Member

@Tpuljak Tpuljak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks great. Nice work! Just please address the 2 small comments I left.

I'll test this on my machine later today and let you know if I encounter any issues.

pkg/provider/provider.go Show resolved Hide resolved
pkg/provider/provider_test.go Outdated Show resolved Hide resolved
Copy link
Member

@Tpuljak Tpuljak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested the provider and it works as expected. Nice work!

There are still a couple of things to iron out that I've commented below.

pkg/provider/provider_test.go Show resolved Hide resolved
pkg/provider/provider.go Show resolved Hide resolved
@Tpuljak
Copy link
Member

Tpuljak commented May 29, 2024

One more thing I noticed.

From the live logs of the machine, it seems that the repo got cloned again after I stopped then started the workspace.

Does this mean that the home folder of the user is not persisted? If so, we will need to make adjustments since it's very important that the git repo persists when a project is restarted.

pkg/provider/util/fly.go Show resolved Hide resolved
@bcmmbaga
Copy link
Collaborator Author

One more thing I noticed.

From the live logs of the machine, it seems that the repo got cloned again after I stopped then started the workspace.

Does this mean that the home folder of the user is not persisted? If so, we will need to make adjustments since it's very important that the git repo persists when a project is restarted.

Machine / path is mounted to the volume but for stopped fly machine when you start the machine are restarted are completely reset to their original state so that they start clean on the next run https://fly.io/docs/machines/api/machines-resource/#start-a-machine

@Tpuljak
Copy link
Member

Tpuljak commented May 29, 2024

Machine / path is mounted to the volume but for stopped fly machine when you start the machine are restarted are completely reset to their original state so that they start clean on the next run https://fly.io/docs/machines/api/machines-resource/#start-a-machine

But volumes should be persistent. Also, you can not mount a volume to a directory that already exists in the image, that's why this "fails".

I suggest you mount the volume to Path: fmt.Sprintf("/home/%s/%s", project.User, project.Name) since that's where the project will be cloned. This should enable at least the git repo to be persisted.

I will discuss with the rest of the team how to address the issue where the rest of the machine is reset to the original state. We will have to make it clear in the README somewhere.

@bcmmbaga
Copy link
Collaborator Author

bcmmbaga commented May 29, 2024

Machine / path is mounted to the volume but for stopped fly machine when you start the machine are restarted are completely reset to their original state so that they start clean on the next run https://fly.io/docs/machines/api/machines-resource/#start-a-machine

But volumes should be persistent. Also, you can not mount a volume to a directory that already exists in the image, that's why this "fails".

I suggest you mount the volume to Path: fmt.Sprintf("/home/%s/%s", project.User, project.Name) since that's where the project will be cloned. This should enable at least the git repo to be persisted.

I will discuss with the rest of the team how to address the issue where the rest of the machine is reset to the original state. We will have to make it clear in the README somewhere.

I have updated the mounted path to /root, which now makes the project persist.

2024-05-29T17:56:01.506 app[e78432e6f23e83] jnb [info] Installing server to /usr/local/bin

2024-05-29T17:56:01.549 app[e78432e6f23e83] jnb [info] Starting Daytona Agent

2024-05-29T17:56:03.803 app[e78432e6f23e83] jnb [info] Repository already exists. Skipping clone...

2024-05-29T17:56:03.804 app[e78432e6f23e83] jnb [info] Running post start commands...

2024-05-29T17:56:03.804 app[e78432e6f23e83] jnb [info] Running command: sudo dockerd

2024-05-29T17:56:03.804 app[e78432e6f23e83] jnb [info] Starting ssh server on port 2222...

@bcmmbaga bcmmbaga force-pushed the add-fly-provider branch 2 times, most recently from bed180c to 229305a Compare May 29, 2024 21:18
pkg/provider/provider_test.go Outdated Show resolved Hide resolved
pkg/provider/util/fly.go Show resolved Hide resolved
pkg/provider/util/fly.go Show resolved Hide resolved
@Tpuljak
Copy link
Member

Tpuljak commented Jun 4, 2024

@bcmmbaga the code looks great now but I'm encountering weird errors again.

Try to create a couple of workspaces in a row. I got this error on the third one:
Screenshot 2024-06-04 at 11 54 48

Seems there's still some race conditions going on.

@bcmmbaga
Copy link
Collaborator Author

bcmmbaga commented Jun 4, 2024

@bcmmbaga the code looks great now but I'm encountering weird errors again.

Try to create a couple of workspaces in a row. I got this error on the third one: Screenshot 2024-06-04 at 11 54 48

Seems there's still some race conditions going on.

I will attempt to reproduce this issue. In the meantime, could you check if an app with this name exists on the Fly dashboard and confirm its status?

@Tpuljak
Copy link
Member

Tpuljak commented Jun 4, 2024

@bcmmbaga the code looks great now but I'm encountering weird errors again.
Try to create a couple of workspaces in a row. I got this error on the third one: Screenshot 2024-06-04 at 11 54 48
Seems there's still some race conditions going on.

I will attempt to reproduce this issue. In the meantime, could you check if an app with this name exists on the Fly dashboard and confirm its status?

The app is always present but remains in Pending state.

@bcmmbaga
Copy link
Collaborator Author

bcmmbaga commented Jun 4, 2024

@bcmmbaga the code looks great now but I'm encountering weird errors again.
Try to create a couple of workspaces in a row. I got this error on the third one: Screenshot 2024-06-04 at 11 54 48
Seems there's still some race conditions going on.

I will attempt to reproduce this issue. In the meantime, could you check if an app with this name exists on the Fly dashboard and confirm its status?

The app is always present but remains in Pending state.

I can confirm it is not a race error, as the error indicates that the machine is included in response from Fly.io. This seems to be related to Fly.io's end, as I have seen similar issues reported by other users when deploying: https://community.fly.io/t/app-stuck-on-pending/9550/13, https://community.fly.io/t/app-stuck-in-pending-state/4680

@Tpuljak
Copy link
Member

Tpuljak commented Jun 4, 2024

I can confirm it is not a race error, as the error indicates that the machine is included in response from Fly.io. This seems to be related to Fly.io's end, as I have seen similar issues reported by other users when deploying: https://community.fly.io/t/app-stuck-on-pending/9550/13, https://community.fly.io/t/app-stuck-in-pending-state/4680

Is it possible to add some sort of check if the app is ready? I can see you implemented WaitForApp but should we check the state of the app as well?

@bcmmbaga
Copy link
Collaborator Author

bcmmbaga commented Jun 4, 2024

I can confirm it is not a race error, as the error indicates that the machine is included in response from Fly.io. This seems to be related to Fly.io's end, as I have seen similar issues reported by other users when deploying: https://community.fly.io/t/app-stuck-on-pending/9550/13, https://community.fly.io/t/app-stuck-in-pending-state/4680

Is it possible to add some sort of check if the app is ready? I can see you implemented WaitForApp but should we check the state of the app as well?

WaitForApp covers this scenario because an app in a pending state won't be returned in the API response.

@Tpuljak
Copy link
Member

Tpuljak commented Jun 4, 2024

WaitForApp covers this scenario because an app in a pending state won't be returned in the API response.

From what I can see, the error is generated from this function:
Screenshot 2024-06-04 at 15 50 14

Should we implement waiting for the app before this?

@bcmmbaga
Copy link
Collaborator Author

bcmmbaga commented Jun 4, 2024

WaitForApp covers this scenario because an app in a pending state won't be returned in the API response.

From what I can see, the error is generated from this function: Screenshot 2024-06-04 at 15 50 14

Should we implement waiting for the app before this?

We can implement this, but we might still encounter the error because the initial check also waits for the app. Since it passed, it means the app was ready, which is why the volume and machine were created. However, just before starting, the app went into a pending state again, making it unrecoverable. In this case, we could experience a timeout while waiting for the app to be ready

@Tpuljak
Copy link
Member

Tpuljak commented Jun 4, 2024

We can implement this, but we might still encounter the error because the initial check also waits for the app. Since it passed, it means the app was ready, which is why the volume and machine were created. However, just before starting, the app went into a pending state again, making it unrecoverable. In this case, we could experience a timeout while waiting for the app to be ready

I suggest that we check the app status again and wait for the appropriate state. If the state isn't reach in 2 minutes, we can notify the user that there was a problem with their app.

Copy link
Member

@Tpuljak Tpuljak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bcmmbaga everything seems to be working great now. Nice work!

Approving this and you can merge yourself once we get one more approval. Please just update the commit message to include the feat: subject (feat: add fly.io provider (#1)). We follow https://www.conventionalcommits.org/en/v1.0.0/ in our org so we want that to be in sync with the rest of our repos.

I'll be opening up an issue immediately that should also be resolved before we release.

@Tpuljak Tpuljak requested a review from vedranjukic June 5, 2024 08:50
Copy link
Member

@vedranjukic vedranjukic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good work!

@bcmmbaga bcmmbaga merged commit 846aa73 into main Jun 5, 2024
2 checks passed
@bcmmbaga bcmmbaga deleted the add-fly-provider branch June 5, 2024 09:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants